Add timeouts and bounded reconnect to Redis client#120
Conversation
Previously the Redis client retried reconnecting forever with no connect or command timeout, so an unreachable Redis made callers (e.g. the OAuth token exchange) block indefinitely instead of failing. Bound the reconnect attempts, set a connect timeout, and cap each command so Redis outages surface as errors. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
Cap ensureConnected() with the same ~5s ceiling as commands (connect drives the reconnect loop, so an unreachable Redis blocked callers for the full reconnect budget), destroy the client on connect timeout so the next call starts clean, swallow late command settlements to avoid unhandled rejections, and fix the reconnect-attempt off-by-one. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want fixes drafted automatically? Bugbot Autofix can create code changes for findings. A team admin can enable Autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit fe21c00. Configure here.
| }); | ||
| connectPromise = connectWithTimeout().finally(() => { | ||
| connectPromise = null; | ||
| }); |
There was a problem hiding this comment.
Parallel connect destroys shared client
Medium Severity
When a Redis connection attempt fails or times out, connectWithTimeout calls client.destroy() on the shared singleton client. This prematurely terminates other in-flight Redis operations or concurrent connection attempts, breaking Redis for those callers.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit fe21c00. Configure here.
|
|
||
| // connect() drives the reconnect loop, so against an unreachable Redis it blocks | ||
| // for the whole reconnect budget. Cap the wait so callers fail within the same | ||
| // ceiling as a command instead of after every reconnect attempt. |
There was a problem hiding this comment.
Late connect after timeout wins
Medium Severity
When the connect deadline wins Promise.race, the code calls client.destroy() but only attaches .catch(() => {}) to the in-flight connect() promise. A late successful connect can still run library side effects, and the global ready listener sets isConnected true, so ensureConnected may skip reconnect while the client was just destroyed.
Reviewed by Cursor Bugbot for commit fe21c00. Configure here.
| } finally { | ||
| if (timer) clearTimeout(timer); | ||
| } | ||
| } |
There was a problem hiding this comment.
Stale state after command timeout
Medium Severity
withTimeout rejects when a command exceeds the deadline but does not clear isConnected or tear down the socket, unlike connectWithTimeout. Later calls can hit ensureConnected's client.isOpen && isConnected early return on a stalled link, so requests keep timing out without attempting a fresh connect.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit fe21c00. Configure here.
2 participants
Summary
The Redis client retried reconnecting forever with no connect or command timeout. When Redis was unreachable, any command (e.g. the OAuth org-context writes in
/token) blocked until the serverless function limit instead of throwing — sokernel loginhung after the browser showed "authentication successful" rather than failing with an error.This bounds the failure:
reconnectStrategynow returns anErrorafterMAX_RECONNECT_ATTEMPTS(10) instead of retrying indefinitely, so queued commands reject when Redis stays down.connectTimeout(5s) on the socket so a black-holed connection fails fast.withTimeout(5s ceiling) so a stalled-but-connected command surfaces as an error.Net effect: a Redis outage returns a clean error in seconds instead of hanging the caller.
Test plan
tsc --noEmitpassesFollow-up companion PR on
kernel/cliadds a timeout on the CLI token exchange so it also fails fast.Note
Medium Risk
Touches shared Redis infrastructure used by OAuth token and authorize flows; mis-tuned timeouts could cause false failures on slow networks, though happy-path behavior is intended to stay the same.
Overview
When Redis is unreachable, OAuth-related callers (e.g. org-context writes on
/token) no longer block until the serverless limit; they fail with an error within a few seconds.Bounded reconnect —
reconnectStrategystops after 10 attempts and returns anErrorinstead of retrying indefinitely.Connect path — 5s
connectTimeouton the socket plusconnectWithTimeout()racingclient.connect()against the same ceiling; on timeout it resets state withclient.destroy()so the next call does not inherit a long reconnect loop.Commands — All Redis ops in
withReconnectrun throughwithTimeout(5s), including the retry after transient socket errors.Reviewed by Cursor Bugbot for commit fe21c00. Bugbot is set up for automated code reviews on this repo. Configure here.